improve time of day transition quality by using Oklab and smoothstep - #509
Merged
Conversation
- Implement sRGB <-> Linear <-> Oklab conversion in timeofday frag shader. - Mix time of day colors in Oklab space for perceptually uniform gradients. - Use `smoothstep` for the time factor in timeofday, atmosphere, and particle shaders to provide non-linear easing during transitions. - Synchronize intensity and emissive boost calculations with the new transition curve.
Reviewer's GuideRefactors time-of-day transitions to use perceptually uniform Oklab color interpolation in the main time-of-day shader and replaces linear time interpolation with smoothstep easing across time-of-day, atmosphere, and particle shaders, while aligning alpha/intensity-based emissive boosts with the new transition curve. Flow diagram for Oklab color mixing in time of day shadergraph TD
A[uNamedColors index uTimeOfDay.x] --> B[timeOfDayStartSrgb]
A2[uNamedColors index uTimeOfDay.y] --> C[timeOfDayTargetSrgb]
B --> D[srgbToLinear start]
C --> E[srgbToLinear target]
D --> F[linearToOklab startOklab]
E --> G[linearToOklab targetOklab]
F --> H[mix in Oklab using timeOfDayLerp]
G --> H
H --> I[oklabToLinear mixed]
I --> J[linearToSrgb mixedSrgb]
subgraph Alpha_and_intensity
B --> K[timeOfDayStartSrgb alpha]
C --> L[timeOfDayTargetSrgb alpha]
K --> M[mix alpha using timeOfDayLerp]
L --> M
M --> N[multiply by currentTimeOfDayIntensity]
end
J --> O[vFragmentColor rgb]
N --> O[vFragmentColor alpha]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The Oklab/sRGB conversion helpers add a fair bit of logic to the time-of-day shader; consider moving these into a shared include or common utility to avoid duplication and keep the main shader focused on the transition logic.
- After converting back from Oklab to sRGB (
mixedSrgb), you may want to explicitly clamp to[0.0, 1.0]to guard against small numerical excursions that could cause banding or artifacts in downstream blending. - The time-of-day transition code now uses slightly different variable naming patterns across shaders (
tvstTod, etc.); aligning these for consistency would make it easier to follow and maintain the transition behavior across the different shader stages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Oklab/sRGB conversion helpers add a fair bit of logic to the time-of-day shader; consider moving these into a shared include or common utility to avoid duplication and keep the main shader focused on the transition logic.
- After converting back from Oklab to sRGB (`mixedSrgb`), you may want to explicitly clamp to `[0.0, 1.0]` to guard against small numerical excursions that could cause banding or artifacts in downstream blending.
- The time-of-day transition code now uses slightly different variable naming patterns across shaders (`t` vs `tTod`, etc.); aligning these for consistency would make it easier to follow and maintain the transition behavior across the different shader stages.
## Individual Comments
### Comment 1
<location path="src/resources/shaders/legacy/weather/timeofday/frag.glsl" line_range="89-90" />
<code_context>
+ vec3 mixedOklab = mix(startOklab, targetOklab, timeOfDayLerp);
+
+ // Convert back to sRGB
+ vec3 mixedSrgb = linearToSrgb(oklabToLinear(mixedOklab));
- float timeOfDayLerp = clamp((uCurrentTime - uTimeOfDayStartTime) / uTransitionDuration,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider clamping the final sRGB color to avoid out-of-gamut values from Oklab interpolation.
Oklab interpolation can yield linear RGB outside [0, 1], which converts to out-of-gamut sRGB. If the pipeline isn’t intentionally handling HDR/overbright values, consider clamping the final sRGB result, e.g.:
```glsl
vec3 mixedSrgb = clamp(linearToSrgb(oklabToLinear(mixedOklab)), 0.0, 1.0);
```
This avoids artifacts in later stages that assume normalized color channels.
```suggestion
// Convert back to sRGB and clamp to valid [0, 1] range to avoid out-of-gamut values
vec3 mixedSrgb = clamp(linearToSrgb(oklabToLinear(mixedOklab)), 0.0, 1.0);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+89
to
+90
| // Convert back to sRGB | ||
| vec3 mixedSrgb = linearToSrgb(oklabToLinear(mixedOklab)); |
There was a problem hiding this comment.
suggestion (bug_risk): Consider clamping the final sRGB color to avoid out-of-gamut values from Oklab interpolation.
Oklab interpolation can yield linear RGB outside [0, 1], which converts to out-of-gamut sRGB. If the pipeline isn’t intentionally handling HDR/overbright values, consider clamping the final sRGB result, e.g.:
vec3 mixedSrgb = clamp(linearToSrgb(oklabToLinear(mixedOklab)), 0.0, 1.0);This avoids artifacts in later stages that assume normalized color channels.
Suggested change
| // Convert back to sRGB | |
| vec3 mixedSrgb = linearToSrgb(oklabToLinear(mixedOklab)); | |
| // Convert back to sRGB and clamp to valid [0, 1] range to avoid out-of-gamut values | |
| vec3 mixedSrgb = clamp(linearToSrgb(oklabToLinear(mixedOklab)), 0.0, 1.0); |
nschimme
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #509 +/- ##
==========================================
- Coverage 25.06% 25.05% -0.01%
==========================================
Files 510 510
Lines 42275 42275
Branches 4574 4572 -2
==========================================
- Hits 10596 10594 -2
- Misses 31679 31681 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
smoothstepfor the time factor in timeofday, atmosphere, and particle shaders to provide non-linear easing during transitions.Summary by Sourcery
Improve time-of-day visual transitions and consistency across weather-related shaders
New Features:
Enhancements: